-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(RELEASE-400): adds check-data-keys task in release pipeline #720
base: development
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
c12a590
to
c7747cd
Compare
d339fc4
to
2e89177
Compare
5d88a5c
to
d7ef3c8
Compare
c9ccb55
to
7ca3e22
Compare
@@ -49,8 +57,7 @@ | |||
}, | |||
"productName": { | |||
"type": "string", | |||
"description": "The product name e.g. exampleproduct ", | |||
"pattern": "^[a-z]+$" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/workspace/data/5c52c6c8-898a-4a85-93b2-9336e6601e52/data.json::$.fbc.productName: 'testProductName' does not match '^[a-z]+$'
Just removing for time being to get E2E to pass.... I would say we can remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe just relax it to allow capital letters? Or modify e2e to contain lowercase in the name? That should be easy. Maybe just this here? https://github.com/konflux-ci/e2e-tests/blob/fd26460f5d0035b0bffe699513c558be8588b209/tests/release/pipelines/fbc_release.go#L347
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hit this with some of the advisory keys too. I am fine with either removing it (the pattern, not the description) or modifying like Martin said so long as it is not too restricting such that users are going to fail on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I have allowed for the pattern to have capital letters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also get this one .fbc.productName: 'preGA-product' does not match '^[a-zA-Z]+$.'
I could add in to allow hyphen or just update e2e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you. I don't know that we have the exact requirements on this field which is why I say we can just remove it, but no strong opinion
7ca3e22
to
a43b87a
Compare
a7d2762
to
7c51620
Compare
5c052db
to
3deabf8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E2E results may be interesting on this one 😂
pipelines/rh-push-to-registry-redhat-io/rh-push-to-registry-redhat-io.yaml
Outdated
Show resolved
Hide resolved
/ok-to-test |
- name: data | ||
workspace: release-workspace | ||
runAfter: | ||
- collect-data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In rh-advisories, this has verify-enterprise-contract in runAfter. Not sure which one is more correct, but they should do the same - and if run after verify-enterprise-contract is correct, then perhaps it should be used in other pipelines that have it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They all should be run after collect-data
... Will update the push-to-cdn
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment is about rh-advisories ;) Not sure what you mean by "will update push-to-cdn" - that's a task. Which pipeline are you talking about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused as rh-advisories
has populate-release-notes-images in its run after for check-data-leys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, and we need to keep it that way AFAIK because it checks some of the keys the populate-release-notes-images task adds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rh-advisories does not have that. Are you looking at the right pipeline? It has this
- name: check-data-keys
params:
- name: dataPath
value: "$(tasks.collect-data.results.data)"
- name: schema
value: $(params.taskGitUrl)/raw/$(params.taskGitRevision)/schema/dataKeys.json
- name: systems
value:
- releaseNotes
taskRef:
params:
- name: url
value: $(params.taskGitUrl)
- name: revision
value: $(params.taskGitRevision)
- name: pathInRepo
value: tasks/check-data-keys/check-data-keys.yaml
resolver: git
workspaces:
- name: data
workspace: release-workspace
runAfter:
- populate-release-notes-images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you looking at the right pipeline?
Thanks. I'm not sure where I was looking. You're right it's not there. Sorry about the noise.
But I do see it in push-disk-images-to-cdn
pipeline:
- name: check-data-keys
params:
- name: dataPath
value: "$(tasks.collect-data.results.data)"
- name: systems
value:
- cdn
taskRef:
params:
- name: url
value: $(params.taskGitUrl)
- name: revision
value: $(params.taskGitRevision)
- name: pathInRepo
value: tasks/check-data-keys/check-data-keys.yaml
resolver: git
workspaces:
- name: data
workspace: release-workspace
runAfter:
- verify-enterprise-contract
Do we know why? Shouldn't it also run right after collect-data there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would imagine it should run right after the collect-data like the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it
8da7b70
to
5cd1579
Compare
1ab75dc
to
e845b07
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, would be nice to get Leandro's input on the fbc keys before merging
e845b07
to
dcdae2c
Compare
This commit adds the check-data-keys task into the release pipelines to ensure validation of required data keys. Signed-off-by: Sean Conroy <sconroy@redhat.com>
dcdae2c
to
a3229d4
Compare
/retest |
1 similar comment
/retest |
@seanconroy2021: The following test has Failed, say /retest to rerun failed tests.
Inspecting Test ArtifactsTo inspect your test artifacts, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/konflux-team/release-service-catalog:konflux-e2e-tests-catalog-xd4wk |
This commit adds the check-data-keys task into the release pipelines to ensure validation of required data keys.
Link